Skip to content

Conversation

@maraf
Copy link
Member

@maraf maraf commented Dec 7, 2020

Fixes #51, an alternative to #52.

image

@maraf maraf requested review from RussKie and mast-eu December 7, 2020 07:30
Copy link
Member

@mast-eu mast-eu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just one comment, but nothing relevant. For me, this is ok.

Would render #52 obsolete.

"Plugin Manager",
"Plugin Manager is going to write to files that are holded by other executables. " + Environment.NewLine +
"Do you want to kill all instances of these applications?"
$"Plugin Manager will be writing to files that are currently in use.\r\n\r\nDo you want to stop {processNames}?"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$"Plugin Manager will be writing to files that are currently in use.\r\n\r\nDo you want to stop {processNames}?"
$"Plugin Manager will be writing to files that are currently in use.\r\n\r\nDo you want to stop all instances of {processNames}?"

This is really just a detail and I have no strong feelings about it. At the end I accept both forms, with and without this suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no option. I'm not good at user texts, as I tend to put there too much information.
Think it's @RussKie's call 😎

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose if there's only a single instance of an app, than it is the first message. Else the second ;))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem 😎

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suggest changing the messagebox's icon to either question or warning.

LGTM

@maraf maraf self-assigned this Dec 10, 2020
@maraf
Copy link
Member Author

maraf commented Dec 14, 2020

Message now contains or don't information about instances based on process count to kill.

@maraf maraf merged commit 4202c83 into gitextensions:master Dec 15, 2020
@maraf maraf deleted the KillMessage branch December 15, 2020 10:03
@maraf maraf mentioned this pull request Dec 15, 2020
@mast-eu mast-eu added this to the 1.2.0 milestone Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect wording

3 participants